Allow staying logged in for longer #4236

davidfischer · 2018-06-13T21:33:03Z

Makes it more obvious that a user won't see ads when they are a donor
Give donors an easy way to stay logged in for longer (3 months)

GOLD MEMBER/DONOR

NON-DONOR

humitos

I like these changes.

It's not still clear to me what's the workflow for this. I suppose once the user gets it's gold membership a one week session is saved. Then the user has to go to this page and increase the session's time.

After those three months, the user will start seeing ads again and it has to do the same process, right?: Go to .org site, log in, go to admin, click this button again or just log in will be enough for next 3 months?

ericholscher · 2018-06-14T08:57:13Z

This feels like a change that nobody will ever find or really use. Can we not catch this when the user logs in, and just extend gold user sessions automatically? Another question -- is there a reason that we have such short login sessions to begin with? Is there a reason not to just leave everyone logged in for 3 months?

agjohnson · 2018-06-14T16:57:36Z

Also going to echo the concern of if we really need this in the UI. I don't think users will find this easily, and I'd like to see what doc-viewing-only user subscription rates look like before optimizing this all.

Is there a view where we can set_expiry on the session to extend this date for a doc-viewing-only user? If we have the session at footer_html, we could update the expiry there.

stsewd · 2018-06-14T17:02:48Z

Maybe related to #3259

davidfischer · 2018-06-14T17:59:16Z

I'd be fine removing the "your login session will expire" part and the "stay logged in" button. My goal is to give gold members a way to not see ads for longer than 2 weeks without having to login constantly.

First, I'll describe our current setup and then I'll discuss options to meet that goal.

Current setup

When logging in, if you click the "remember me" checkbox, your login session will last for 2 weeks. 2 weeks is the Django default.
Merely visiting the website within that 2 week time period does not refresh the 2 week time period. However, anything that writes to request.session will restart a new 2 week window. We don't have anything that does that though.
If you do not click the "remember me" checkbox, your login cookie is a session cookie and is deleted when the browser closes. You'll need to login again the next time you open your browser.

Possible options

Increase the login cookie expiry. What is a reasonable expiry might depend on whether we combine this with any options below. I think 30 days at a minimum is fine.
Save the session cookie on every request. This is a setting in Django and is as simple as setting a flag. This means that anytime somebody visits readthedocs.org (including via the footer API call on docs pages) their session cookie expiry will refresh. This will involve an extra cache write (our production session settings use the cache) in addition to the existing cache read for the session on every request.
We could detect a gold member in the advertising API call (we already do this) and refresh their session cookie expiry.

My recommendation

Remove the "your login session will expire" part and the "stay logged in" button on the advertising screen
Change the login cookie expiry to 30 days
Save the session on every request

This means that somebody who visits readthedocs.org (including via the footer call) will never need to login again as long as they visit once every 30 days. Anybody who doesn't click the "remember me" button will still logout when they close their browser.

This would solve #3259 and reduce the frequency that .com users need to login.

Side note

I have a preference that our production session settings used signed cookies as opposed to cache based sessions. Since we don't write much of anything to the session there are almost no downsides and the upside is to avoid having to connect to the cache on every request which we do now. It would also free up cache space. Making this change though would log everyone out which isn't a big deal right now since people get logged out every 2 weeks anyway.

ericholscher · 2018-06-15T10:35:38Z

I'm +1 on the suggested implementation (30 days & re-uping on request). It seems like moving to signed cookies would be a nice performance win too. Is there anything else on the footer call that depends on the cache, or would this allow us to continue serving footer requests even when the cache is down? IIRC we hit it in middleware for CNAME's, and perhaps a couple other places, but reducing the usage of it makes a lot of sense to me.

davidfischer · 2018-06-15T18:44:57Z

I made the changes to remove the UI elements to extend the log in session and just made them the defaults.

30 day session cookie expiry
resave the session on every request (the 30 day expiry becomes 30 days after the last request)
Switch to signed cookie sessions

NOTE: Deploying this will log everyone out although after that they will stay logged in much longer.

Fixes #3259

Is there anything else on the footer call that depends on the cache

There doesn't appear to be but advertising does rely on the cache.

stsewd · 2018-06-15T18:50:11Z

readthedocs/profiles/views.py

@@ -310,5 +310,6 @@ def account_advertising(request):
            'form': form,
            'profile': profile_obj,
            'user': profile_obj.user,
+            'session_expiry': request.session.get_expiry_date(),


I believe this isn't used with the recent changes

humitos

These changes make sense to me and will help Gold users to not see ads for a long time :)

I took a look at other places where we use sessions and I found this middleware that do not creates a session for non-logged in users when hitting the footer. I think it stills makes sense, but wanted to mention to you just in case you see that something need to be touched there: https://github.com/rtfd/readthedocs.org/blob/c87e646413dcd7e9f66f733b559d10010c3fa3db/readthedocs/core/middleware.py#L222

humitos · 2018-06-18T13:19:46Z

readthedocs/settings/base.py

    SESSION_COOKIE_DOMAIN = 'readthedocs.org'
    SESSION_COOKIE_HTTPONLY = True
+    SESSION_COOKIE_AGE = 30 * 24 * 60 * 60  # 30 days
+    SESSION_ENGINE = 'django.contrib.sessions.backends.signed_cookies'


If we are going to use signed cookies, don't we want to remove django.contrib.sessions from INSTALLED_APPS?. What I understand from the documentation it's only used for database-cookies.

https://docs.djangoproject.com/en/1.11/topics/http/sessions/#using-database-backed-sessions

I'm thinking of actually taking this change out and making this a setting that is set in the production settings rather than in the base settings.

There are advantages to signed cookies but they aren't huge and they are what's holding this up due to the complications with the corporate site.

I removed the SESSION_ENGINE setting here. If we decide to change the production SESSION_ENGINE, we'll do that in the production settings file. This will ensure that anyone taking our code and using it don't have a SESSION_ENGINE change unless they want one.

Makes sense. 👍

ericholscher

Looks good.

davidfischer · 2018-06-18T16:28:23Z

No changes should be required by the FooterNoSessionMiddleware

Allow staying logged in for longer

0e5f4b1

davidfischer requested a review from a team June 13, 2018 21:33

humitos reviewed Jun 14, 2018

View reviewed changes

agjohnson added Improvement Minor improvement to code Needed: design decision A core team decision is required labels Jun 14, 2018

davidfischer added 2 commits June 15, 2018 11:25

Remove extending session cookie

3f3358f

Update session settings

ef99a49

stsewd reviewed Jun 15, 2018

View reviewed changes

Remove session expiry variable that isn't used

debde2f

humitos approved these changes Jun 18, 2018

View reviewed changes

ericholscher approved these changes Jun 18, 2018

View reviewed changes

Don't set the production session engine here

d7f8b6a

ericholscher merged commit 08591ea into master Jun 22, 2018

humitos deleted the davidfischer/advertising-preferences-tweak branch June 22, 2018 13:42

humitos mentioned this pull request Jan 7, 2019

Extend lifetime of sessionid cookie on every usage #3259

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow staying logged in for longer #4236

Allow staying logged in for longer #4236

davidfischer commented Jun 13, 2018

humitos left a comment

ericholscher commented Jun 14, 2018

agjohnson commented Jun 14, 2018

stsewd commented Jun 14, 2018

davidfischer commented Jun 14, 2018

ericholscher commented Jun 15, 2018

davidfischer commented Jun 15, 2018

stsewd Jun 15, 2018

humitos left a comment

humitos Jun 18, 2018

davidfischer Jun 21, 2018

davidfischer Jun 21, 2018

davidfischer Jun 21, 2018

ericholscher Jun 22, 2018

ericholscher left a comment

davidfischer commented Jun 18, 2018

Allow staying logged in for longer #4236

Allow staying logged in for longer #4236

Conversation

davidfischer commented Jun 13, 2018

GOLD MEMBER/DONOR

NON-DONOR

humitos left a comment

Choose a reason for hiding this comment

ericholscher commented Jun 14, 2018

agjohnson commented Jun 14, 2018

stsewd commented Jun 14, 2018

davidfischer commented Jun 14, 2018

Current setup

Possible options

My recommendation

Side note

ericholscher commented Jun 15, 2018

davidfischer commented Jun 15, 2018

stsewd Jun 15, 2018

Choose a reason for hiding this comment

humitos left a comment

Choose a reason for hiding this comment

humitos Jun 18, 2018

Choose a reason for hiding this comment

davidfischer Jun 21, 2018

Choose a reason for hiding this comment

davidfischer Jun 21, 2018

Choose a reason for hiding this comment

davidfischer Jun 21, 2018

Choose a reason for hiding this comment

ericholscher Jun 22, 2018

Choose a reason for hiding this comment

ericholscher left a comment

Choose a reason for hiding this comment

davidfischer commented Jun 18, 2018